-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Sync extension bridge settings with cloud #7506
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution! I've reviewed the changes and found several issues that need attention before this can be merged.
|
|
||
| const remoteControlEnabled = isCloudAgent | ||
| ? true | ||
| : (CloudService.instance.getUserSettings()?.settings?.extensionBridgeEnabled ?? false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intentional? The error handling here only logs the error but doesn't provide any fallback value for remoteControlEnabled. If getUserSettings() returns null/undefined, the remote control state could be out of sync.
Consider ensuring the fallback value is always applied.
|
|
||
| const remoteControlEnabled = isCloudAgent | ||
| ? true | ||
| : (CloudService.instance.getUserSettings()?.settings?.extensionBridgeEnabled ?? false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic for determining remoteControlEnabled is duplicated from lines 144-146. Could we extract this into a helper function to avoid duplication?
For example:
function getRemoteControlEnabled(isCloudAgent: boolean): boolean {
return isCloudAgent ? true : (CloudService.instance.getUserSettings()?.settings?.extensionBridgeEnabled ?? false);
}| let remoteControlEnabled: boolean = false | ||
|
|
||
| try { | ||
| const cloudSettings = CloudService.instance.getUserSettings() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing null check - what happens if CloudService is not initialized yet? The code uses optional chaining but doesn't verify that CloudService.instance exists.
Consider adding a hasInstance() check before accessing the instance.
| extensionBridgeEnabled: message.bool ?? false, | ||
| }) | ||
| } catch (error) { | ||
| provider.log(`Failed to update cloud settings for remote control: ${error}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error is only logged but doesn't provide user feedback. Should we show an error message to the user when cloud settings fail to update?
Consider adding a user-facing error notification.
Important
Sync
remoteControlEnabledsetting with cloud, making cloud the source of truth, and improve error handling for cloud interactions.remoteControlEnabledsetting inClineProvider.tsnow fetched from cloud settings usingCloudService.instance.getUserSettings().webviewMessageHandler.ts, updates toremoteControlEnablednow update cloud settings viaCloudService.instance.updateUserSettings().extension.tsupdatesExtensionBridgeServicewithremoteControlEnabledfrom cloud settings.ClineProvider.tsandextension.ts.remoteControlEnabledinClineProvider.ts.This description was created by
for dace718. You can customize this summary. It will automatically update as commits are pushed.